Skip to content

Comments

[SYCL] Finally fix iostream_proxy.hpp#6517

Merged
pvchupin merged 1 commit intointel:syclfrom
aelovikov-intel:io_fix
Aug 3, 2022
Merged

[SYCL] Finally fix iostream_proxy.hpp#6517
pvchupin merged 1 commit intointel:syclfrom
aelovikov-intel:io_fix

Conversation

@aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Aug 2, 2022 •
edited
Loading

When _MT and _DLL are both defined, the user has passed /MD or /MDd and
is dynamically linking in the runtimes. Because std::cout etc are data,
they need to be marked dllimport in that case or else the linker cannot
find them.

@aelovikov-intel aelovikov-intel requested a review from a team as a code owner August 2, 2022 17:19
@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Aug 2, 2022

Many thanks to @AaronBallman for helping with this!

AaronBallman
AaronBallman reviewed Aug 2, 2022
View reviewed changes
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, these changes LGTM.

For reference, when _MT and _DLL are both defined, the user has passed /MD or /MDd and is dynamically linking in the runtimes. Because std::cout etc are data, they need to be marked dllimport in that case or else the linker cannot find them.

@pvchupin
Copy link
Contributor

pvchupin commented Aug 2, 2022

when _MT and _DLL are both defined, the user has passed /MD or /MDd and is dynamically linking in the runtimes. Because std::cout etc are data, they need to be marked dllimport in that case or else the linker cannot find them.

This should be in PR description I think.

@bader bader mentioned this pull request Aug 3, 2022
Merged
@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Aug 3, 2022

I believe

Failed Tests (2):
  SYCL :: Assert/assert_in_simultaneously_multiple_tus.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp

are known and there was/were issue(s) submitted for that. @intel/llvm-gatekeepers , ready to merge?

@pvchupin
Copy link
Contributor

pvchupin commented Aug 3, 2022

I've restarted testing yesterday to check if it's really flaky and it didn't help. Pushed one more restart today... just for curiosity...
I don't think fail is related to the change because it's NFC on Linux it seems.

#6463 looks related.
There is also #6520 for HIP.

@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Aug 3, 2022

Failing test is being disabled at intel/llvm-test-suite#1127.

@pvchupin pvchupin merged commit 62c36e9 into intel:sycl Aug 3, 2022
raaiq1 pushed a commit to raaiq1/llvm that referenced this pull request Aug 4, 2022
@aelovikov-intel @raaiq1
[SYCL] Finally fix iostream_proxy.hpp (intel#6517)
765de0b 
When _MT and _DLL are both defined, the user has passed /MD or /MDd and
is dynamically linking in the runtimes. Because std::cout etc are data,
they need to be marked dllimport in that case or else the linker cannot
find them.
@aelovikov-intel aelovikov-intel deleted the io_fix branch November 8, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AaronBallman AaronBallman AaronBallman left review comments

@sergey-semenov sergey-semenov sergey-semenov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aelovikov-intel @pvchupin @AaronBallman @sergey-semenov